Skip to content

Abstract out OTLP http exporter logic#5160

Open
lorenzoronzani wants to merge 19 commits intoopen-telemetry:mainfrom
lorenzoronzani:refactor/http-exporter-logic
Open

Abstract out OTLP http exporter logic#5160
lorenzoronzani wants to merge 19 commits intoopen-telemetry:mainfrom
lorenzoronzani:refactor/http-exporter-logic

Conversation

@lorenzoronzani
Copy link
Copy Markdown

@lorenzoronzani lorenzoronzani commented Apr 29, 2026 •
edited
Loading

Description

This PR contains the refactoring required inside the http_exporter_logic component. The idea is to remove the duplicated code abstracting out inside a common section

Fixes #2990

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (functionalities are the same, interfaces also but the logic is moved inside codebase)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Executed component specific unit tests using the following command uv run tox -e py312-test-opentelemetry-exporter-otlp-proto-http

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No. No, it's just an internal refactoring

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added/updated
  • Documentation has been updated, this is a pure internal refactor, not interfaces update so I don't think that a documentation update it's required

@lorenzoronzani lorenzoronzani requested a review from a team as a code owner April 29, 2026 07:20
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 29, 2026 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 29, 2026

/easycla

@lorenzoronzani lorenzoronzani force-pushed the refactor/http-exporter-logic branch from 2a75552 to 4405d3e Compare April 29, 2026 08:11
DylanRussell
DylanRussell reviewed Apr 29, 2026
View reviewed changes
Comment thread ...etry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py Outdated
Comment thread ...etry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py Outdated
Comment thread ...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py Outdated
herin049
herin049 requested changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@herin049 herin049 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@github-project-automation github-project-automation Bot moved this to Reviewed PRs that need fixes in Python PR digest Apr 30, 2026
@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 30, 2026

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

I was thinking the same but looking around I thought that the implemented method was the one expected. I can convert with a common client class.

Thanks for the review

@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 30, 2026

I moved these 3 public constants:

DEFAULT_COMPRESSION
DEFAULT_ENDPOINT
DEFAULT_TIMEOUT

from each exporter (Log, Metric, Trace) into the common one but this uv run tox -e public-symbols-check check now it's failing.

Q: What is the right process in this cases?

@lorenzoronzani lorenzoronzani requested a review from herin049 April 30, 2026 12:48
DylanRussell
DylanRussell reviewed Apr 30, 2026
View reviewed changes
Comment thread ...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py
break
return LogRecordExportResult.FAILURE
return (
LogRecordExportResult.SUCCESS
Copy link
Copy Markdown
Contributor

@DylanRussell DylanRussell Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this Enum be part of _SignalConfig and returned directly from OTLPHttpClient

DylanRussell
DylanRussell reviewed Apr 30, 2026
View reviewed changes
Comment thread CHANGELOG.md

## Unreleased

- `opentelemetry-exporter-otlp-proto-http`: refactor shared HTTP exporter logic into common module, extract `_setup_session`, `_export`,
Copy link
Copy Markdown
Contributor

@DylanRussell DylanRussell Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is sort of obsolete now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DylanRussell DylanRussell DylanRussell left review comments

@herin049 herin049 Awaiting requested review from herin049

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Reviewed PRs that need fixes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Abstract out OTLP http exporter logic

3 participants

@lorenzoronzani @DylanRussell @herin049